Address inconsistent behavior on flux_led component by oblogic7 · Pull Request #14713 · home-assistant/core

oblogic7 · 2018-05-31T02:27:42Z

Description:

Corrects inconsistent behavior between different versions of Magic Home controllers that was introduced as part of recent PR to separate white level on the flux_led component. Rigorous testing done on two types of Magic Home LED controllers to confirm that behavior is now consistent.

Also adds white only mode that remaps white value to use brightness for instances where only W channel is connected on RGBW controller. This should restore compatibility with Homekit or other components that expect brightness to control white level in these cases.

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5470

Correct issue with comparison that was preventing white value slider from being shown.

tadly · 2018-05-31T10:21:12Z

Was about to search for the previous PR to report some issue.

For me, with an rgbw controller, turning it on resets the white channel to 0.
In other words it does not "remember" it's previous value(s) (not sure if this applies to other properties as well).

Will this be fixed with this PR or should I do some more investigating?

oblogic7 · 2018-05-31T11:56:35Z

That is fixed with this PR.

…

On Thu, May 31, 2018 at 5:21 AM tadly ***@***.***> wrote: Was about to search for the previous PR to report some issue. For me, with an rgbw controller, turning it on resets the white channel to 0. In other words it does not "remember" it's previous value(s) (not sure if this applies to other properties as well). Will this be fixed with this PR or should I do some more investigating? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14713 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbs0D0Oq2lzK5uaNdcf-ueRz2KW3HApks5t38Q3gaJpZM4UUUZT> .

mikebsec · 2018-06-01T05:12:00Z

There is one remaining issue here. White only is a valid use of this controller: https://www.amazon.com/gp/product/B01DY56N8U When wired as white only it no longer changes brightness by the brightness slider, it controls via the white_value slider. That is extremely non-intuitive to someone who can't wade through these merges/PRs. Perhaps it is time to add a "mode: white" to flux_led and properly map white_value <-> brightness for that use case? That would also make it work transparently with homekit. (all of which worked before the changes started on this train) I can always create a template light to accomplish this for myself, but good luck anyone in the future ever figuring that out. Thoughts?

tadly · 2018-06-01T08:45:41Z

@mikebdotorg you can already set mode to one of rgb and rgbw (rgbw being default)
See the docs here

Also, for rgbw controllers you need both sliders as you can control the brightness for the rgb and the w part independently.

mikebsec · 2018-06-01T12:53:19Z

@tadly - Understood for someone that has 'rgb' AND 'w' wires coming out of their controller that need to be modified.

However, the scenario I am describing here is if someone ONLY wires the 'w' channel. The removal of self._bulb.setWarmWhite255(brightness) with PR 13985 does not make sense intuitively. If someone shows up to homeassistant and sets up a white light but is told that brightness doesn't control brightness that's going to be an odd discussion. The Magic Home app itself does this intuitively for the user who only has white setup so it seems odd homeassistant wouldn't do the same.

I'm suggesting for those users that a "mode: white" is created and there is only a single slider called brightness as all other controls do not serve a purpose any longer. There is no rgb wired to control. For those users this change has broken functionality (automations, homekit, etc... that controlled brightness now fail to do so).

mikebsec · 2018-06-01T12:58:28Z

Magic Home for white only for posterity.

oblogic7 · 2018-06-01T14:30:55Z

I added white mode for cases like @mikebdotorg described and tested with the controller he linked. Seems to be working well for me, but would be great if somebody else would kick the tires.

Example config:

    192.168.1.10:
      name: Test LED 2
      mode: "w"

mikebsec · 2018-06-01T14:57:31Z

@oblogic7 - thank you! I'll test tonight (MDT) and let you know.

mikebsec · 2018-06-02T03:03:05Z

@oblogic7 - worked great. All functionality as expected. Can dim with brightness slider on white only in hass UI, and with homekit. Config as you stated:

      192.168.1.11:
        name: left_poster
        mode: "w"

Thanks again. Looking forward to this being merged.

kineticscreen · 2018-06-06T07:42:09Z

I just noticed something else (I'm running 0.70.1)

When you turn the lights on via hassio, they snap on immediately, rather than fade up.
Turning them off has the correct fade out action.

This is only true via home assistant - the Magic Wifi App has the fade behaviour for both on and off events.

pezinek · 2018-06-06T15:27:39Z

+
+        # handle RGB mode
+        else:
+            self._bulb.setRgb(*tuple(rgb), brightness=brightness)


Could you confirm that calling turn_on(rgb=[5,0,0], brighness=None) will really set the RGB color to [5,0,0] and not to [255,0,0] or anything else deformed by the previous brightness settings you re-use on line 254 ?

Finally had a chance to test this and I found some inconsistent behavior. However, it appears that it is not with the handling of the values by the flux component, but some issue with the way the values are passed into turn_on.

I used the following payload to test:

{ "entity_id": "light.test_led_1", "rgb_color": [0,5,0] }

rgb_color is being converted to hs_color before being passed into the turn_on method. The flux component then uses color_util.color_hs_to_RGB to convert into a tuple of rgb values. This conversion results in 0,255,0 for the payload above instead of the expected 0,5,0. This is the source of the inconsistent behavior that I'm experiencing.

The flux library will only recalculate rgb values if a numeric value is passed for brightness. If None is passed, then it does not recalculate. I am making a change so that existing brightness value is only passed if rgb is None. This should prevent unexpected recalculation of the brightness when brightness is not included on the payload.

I'm not familiar enough with HA internals to know where to fix the hs_color bug mentioned above.

@oblogic7: Thanks for investigating furhter !
It seems that this breakage that changes rgb_color [0,5,0] to [0,255,0] has been introduced in PR #11288.
@armills: any clues/suggestions how to fix that ?

After digging through the discussion on #11288, I see that @amelchio calls out this bug specifically.

https://github.com/home-assistant/home-assistant/pull/11288/files/1ee6a9f786c0508a7079aa2eaf282e03b2b83c9d#r175284144

I'm still checking out the changes, but I don't think the fix will be implemented on components unless there have been updates since that PR was merged. Maybe a new method has been added that I haven't found yet? If not, it looks to me like it will require more changes to the conversion methods.

Popping in because I was tagged. I did not read all of these comments and I don't have flux_led hardware. Still, if you have specific questions about how things should work, I might be able to help.

@amelchio Are you aware of a fix or work around for the issue that you point out here: https://github.com/home-assistant/home-assistant/pull/11288/files/1ee6a9f786c0508a7079aa2eaf282e03b2b83c9d#r175284144

If not, do you know if it will be a component level change or if it will be more involved?

After #11288, the intention is that rgb_color no longer changes the brightness. So turn_on(rgb=[5,0,0], brighness=None) and turn_on(rgb=[255,0,0], brighness=None) should act in the same way: set the light to red with the current brightness level.

This must be implemented in each light platform, like flux_led.py in this case, and

if brightness is None: brightness = self.brightness

seems like a reasonable way to achive that.

Great. That is how it was implemented. I will roll back the last change so that the current brightness is used. Thanks!

pezinek · 2018-06-06T15:39:08Z

... as well would you mind to update the tests so they check the behaviour you broke in your previous PR's and are trying to fix in this one ?

oblogic7 · 2018-06-18T01:31:09Z

@pezinek The differences were in the library methods that were being used and different versions of the controllers handling the calls differently. I'm not sure what decreased the coverage though. Will check that out.

…mediate on action

…alculation of passed rgb values.

amelchio · 2018-06-23T12:27:13Z

+            self._bulb.setRgb(*tuple(rgb), brightness=brightness)
+
+        if not self.is_on:
+            self._bulb.turnOn()


Can you confirm that self._bulb.getRgb() works even when the bulb is not turned on?

Yes, it does.

amelchio

Looks good.

It seems that all comments have been addressed so I will merge this tomorrow if nobody objects.

kineticscreen · 2018-06-26T00:59:05Z

Seems this didn't make it into 0.72.1

amelchio · 2018-06-26T19:27:35Z

Now merged. This is tricky enough that it should get some beta testing so I will not tag it for a hotfix.

kineticscreen · 2018-07-08T06:44:28Z

Well done, this mostly works.

The only problem I'm observing is that the units when sent a 'scene state' from being off, they ignore the RGB value, and just turn on to their previous state.

A second hit of the scene changes the RGB to the correct values.

tadly · 2018-07-08T08:24:48Z

I've also noticed one more nitpick.

If you have a RGBW controller and:

have brightness > 0
have white value > 0
use the slider to bring brightness down to 0
=> the light switch turns off

In other words, only if both sliders are at 0 the switch should turn off

oblogic7 · 2018-07-08T13:49:53Z

@kineticscreen Can you post the payload you are using for the service call? Maybe it is related to this?

@tadly This is due to the way that HA couples brightness to on/off. When brightness is 0, HA calls turn_off. I did run into this when making the updates, but could not tell the difference between turn_off being called from the UI or service call vs when HA was calling it because brightness was 0.

MartinHjelmare · 2018-07-08T14:45:04Z

Please continue troubleshooting in an issue. We don't want discussion or troubleshooting in closed PRs.

Address inconsistent behavior between different controllers.

28221ae

Correct issue with comparison that was preventing white value slider from being shown.

homeassistant added cla-signed platform: light.flux_led small-pr PRs with less than 30 lines. labels May 31, 2018

oblogic7 mentioned this pull request May 31, 2018

Flux bug fix #14476

Merged

2 tasks

Add white mode for Flux LED

5877753

homeassistant added the cla-signed label Jun 1, 2018

oblogic7 mentioned this pull request Jun 1, 2018

Add white mode for Flux LED home-assistant/home-assistant.io#5470

Merged

2 tasks

pezinek reviewed Jun 6, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/dev' into flux_bug_fix_2.1

4cd29fb

homeassistant added the cla-signed label Jun 18, 2018

ghost added the in progress label Jun 18, 2018

oblogic7 added 2 commits June 19, 2018 08:40

Call _bulb.turnOn() after bulb properties have been set to prevent im…

0ca1c40

…mediate on action

Only use existing brightness if rgb is None to prevent unexpected rec…

0cedda0

…alculation of passed rgb values.

homeassistant added the cla-signed label Jun 19, 2018

Remove blank line

2cd2265

homeassistant added the cla-signed label Jun 20, 2018

Undo change so current brightness is used in all cases.

d3800ee

homeassistant added the cla-signed label Jun 22, 2018

amelchio reviewed Jun 23, 2018

View reviewed changes

amelchio approved these changes Jun 25, 2018

View reviewed changes

amelchio merged commit 15af6b1 into home-assistant:dev Jun 26, 2018

ghost removed the in progress label Jun 26, 2018

balloob mentioned this pull request Jul 6, 2018

0.73 #15330

Merged

home-assistant locked and limited conversation to collaborators Jul 8, 2018

ghost added integration: flux_led and removed platform: light.flux_led labels Mar 21, 2019

Uh oh!

Conversation

oblogic7 commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

tadly commented May 31, 2018

Uh oh!

oblogic7 commented May 31, 2018 via email

Uh oh!

mikebsec commented Jun 1, 2018

Uh oh!

tadly commented Jun 1, 2018

Uh oh!

mikebsec commented Jun 1, 2018

Uh oh!

mikebsec commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oblogic7 commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikebsec commented Jun 1, 2018

Uh oh!

mikebsec commented Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kineticscreen commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pezinek Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pezinek commented Jun 6, 2018

Uh oh!

oblogic7 commented Jun 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amelchio left a comment

Choose a reason for hiding this comment

Uh oh!

kineticscreen commented Jun 26, 2018

Uh oh!

amelchio commented Jun 26, 2018

Uh oh!

kineticscreen commented Jul 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tadly commented Jul 8, 2018

Uh oh!

oblogic7 commented Jul 8, 2018

Uh oh!

MartinHjelmare commented Jul 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

oblogic7 commented May 31, 2018 •

edited

Loading

mikebsec commented Jun 1, 2018 •

edited

Loading

oblogic7 commented Jun 1, 2018 •

edited

Loading

mikebsec commented Jun 2, 2018 •

edited

Loading

kineticscreen commented Jun 6, 2018 •

edited

Loading

pezinek Jun 6, 2018 •

edited

Loading

kineticscreen commented Jul 8, 2018 •

edited

Loading